v1.17: BankingStage Forwarding Filter (backport of #685)#696
Conversation
|
Cherry-pick of 1744e9e has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
tao-stones
left a comment
There was a problem hiding this comment.
backport looks good, but the CI failed on the same spot of local-cluster test. I restarted it, see if it's just flaky.
|
@steviez I removed |
Looks like the test is verifying we can clean up the ledger at the expected rate (i.e. purge slots). Expectation is that the max slots we have is whatever we set in the validator config + num validators. We're seeing more than 3x that during this test. No idea why this change would impact this. Is this test consistently passing w/o these changes? |
|
|
||
| #[test] | ||
| #[serial] | ||
| fn test_ledger_cleanup_service() { |
There was a problem hiding this comment.
Yeah, see comment here: #696 (comment)
This test passed rather consistently without change, on my local runs |
Ok, so digging up git history which I'm sure y'all already did, I removed the test in this PR solana-labs#33947 with this justification:
I think it is probably fine to remove, but let me recreate the failure locally and see if I can figure it out real quick just to be 100% sure |
|
Even with the tip of However, with this branch with the test deletion reverted, I've seen I still think this test isn't great in that it is testing something in an overly complex way (don't need a local cluster to test that blockstore functionality), but it did seem to reveal some underlying change of behavior that we should probably understand before pushing the PR |
2793b3b to
ce0d46f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v1.17 #696 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 806 806
Lines 219284 219295 +11
=======================================
+ Hits 179014 179050 +36
+ Misses 40270 40245 -25 🚀 New features to boost your workflow:
|
steviez
left a comment
There was a problem hiding this comment.
LGTM, glad we took the time to figure out the local cluster failures
Problem
Summary of Changes
Fixes #
This is an automatic backport of pull request #685 done by [Mergify](https://mergify.com).